Add Eio.Vars.t to Stdenv by patricoferris · Pull Request #923 · ocaml-multicore/eio

patricoferris · 2026-08-19T11:42:19Z

Access to environment variables after some discussion in #922

avsm

Some high level thoughts:

getenv/putenv aren't multithreaded safe, so is this a good opportunity to provide domain safety by introducing a mutex, so that if accesses go via Eio.Vars they are synchronised? We cant do anything about direct uses of Unix but this is better than nothing
bear in mind that secure_getenv will result in a null if setuid binary, but I think this is handled at the Unix library level.
The Eio.Vars layer is where we should be dealing with portability, so a really useful future addition would be to add a PATH parser using our shiny new Nt_path and Posix_path modules. On Windows it's ; separated and on POSIX : separated.

patricoferris · 2026-08-19T18:24:38Z

Thanks for the review Anil!! All very reasonable to me and I'll incorporate this soon!

patricoferris · 2026-08-20T10:33:14Z

Re:threadsafety-- do you think, in practice, this is a big issue? It seems that the functions are mostly not thread safe when used together across domains. But are a lot of programs doing a lot of "putting"?

avsm · 2026-08-20T10:56:36Z

I think the putting mainly happens at start-of-day. I do this, for example, when linking to third party libraries who need environmental configuration. It also happens most obviously at exec time but that's covered by other code.

patricoferris · 2026-08-21T11:15:32Z

I think the putting mainly happens at start-of-day. I do this, for example, when linking to third party libraries who need environmental configuration. It also happens most obviously at exec time but that's covered by other code.

If it happens at start of day, do you think there's much chance of hitting race conditions with other domains?

6b4f6d7 adds get_path and put_path functions.

avsm · 2026-08-21T18:10:16Z

If it happens at start of day, do you think there's much chance of hitting race conditions with other domains?

I think if something's exposed in the API, it should be as safe as possible. If we're exposing putenv, then why not make it safe with a mutex? If we think it's not used, then it can also just not be included, which I'd prefer to a racy one. Unix.putenv still exists in practise for that.

patricoferris · 2026-09-01T12:46:15Z

I think if something's exposed in the API, it should be as safe as possible. If we're exposing putenv, then why not make it safe with a mutex?

That's fair enough. I have added a mutex in the eio_unix implementation which the three backends use: 8872eea

avsm

This looks good to me! I took the liberty of pushing two changes:

c20fae7 allocates the mutex once per stdenv, as otherwise each invocation of vars won't synchronise
15018f8 makes ~sep a char for symmetry

talex5 · 2026-09-02T09:38:10Z

+val get_all : _ t -> (string * string) list
+(** [get_all vars] gets the full list of environment variables. *)
+
+val get : _ t -> string -> string


I suggest either making this get_opt, or raising a more useful Eio.Io exception (giving the name of the variable that was missing).

talex5 · 2026-09-02T09:49:27Z

+val get_path : _ t -> string list
+(** [get_path var] gets the ["PATH"] variable and returns the paths as a list.
+
+    @raise Not_found if ["PATH"] does not exist.*)


I'm not very keen on this. There are lots of PATH-like variables (e.g. MANPATH). Probably this should take the variable name too.

Ok, maybe something like?

val get_paths : _ t -> string -> string list (** [get_paths var name] gets the value associated with [name] and splits it using the backend-specific path separator. *)

That looks good to me. Probably also worth specifying what happens with repeated separators -- are empty segments preserved? (I'm not even sure if that's valid in PATHs)

talex5 · 2026-09-02T09:55:44Z

+
+let try_get t name =
+  try
+    Eio.traceln "%s is %a" name Fmt.(quote string) (Eio.Vars.get t name)


Suggested change

Eio.traceln "%s is %a" name Fmt.(quote string) (Eio.Vars.get t name)

Eio.traceln "%s is %S" name (Eio.Vars.get t name)

shorter, and works correctly with strings containing quotes.

talex5 · 2026-09-02T09:57:49Z

+module Vars = Vars
+(** An {! Eio.System} interface using standard {! Unix} functions. *)


Should be in Private (and remove the System bit, or the whole comment).

I'm not even sure it needs to be a module; we could just have Private.vars be the actual resource. It can work out the path separator for itself easily enough from Sys.os_type. Then each backend can just do method vars = Eio_unix.Private.vars.

talex5 · 2026-09-02T09:59:49Z

 (** Managing child processes. *)
 module Process = Process

+(** {2 Environment Variables} *)


Could leave out the heading. Putting environment variables under Processes seems fine.

talex5 · 2026-09-02T10:07:37Z

+    val get : t -> string -> string
+    val get_path : t -> string list
+    val put : t -> name:string -> value:string -> unit
+    val put_path : t -> string list -> unit


setenv probably makes more sense for the name than putenv here (in C, setenv is the newer one).

Would be good to have unsetenv too.

talex5 · 2026-09-02T10:12:23Z

+
+(** {1 Accessing environment variables} *)
+
+val get_all : _ t -> (string * string) list


It's not very helpful that get_all returns a list of pairs, but Process.spawn ~env expects an array of strings.

The reasoning here was that I saw a few distinct use cases with Process.spawn ~env:

The user wants to inherit the environment from the parent in which case they provide no value to ~env, so there is no need for get_all to agree on the type.

The user wants to provide a filtered set of the parent process's environment variables in which case they are likely going to want to have access to (name, value) pairs in order to do the filtering before having to rejoin them to be a string array. Of course, some filtering could be done on a version of get_all that returns a string array, for example, String.starts_with ~prefix:"OCAML" v.

The user wants to extend the variables to include everything from the parent in addition to some executable-specific environment variables: this is the most compelling case for get_all to return a string array.

This does not take into account other use cases for get_all like finding some environment variables and their values for in-process OCaml logic where, I think, (name, value) pairs make the most sense.

I'm happy to make this return a string array (and we could provide a split function) but I thought I would lay out some of the reasoning for the current API.

Lemme know what you think :-)

If anything, this is case to change the Process.spawn API to take a list instead. It's a bit of a throwback to the C API to access an OCaml array here

I've opened #930, which changes the type spawn uses to Process.Env.t. Currently it's an alias of string array, but we can change it later once people have a chance to start using the new functions in there.

get_all can then use the same type. Though possibly it should just be Process_mgr.current_environment or something. The process manager already implicitly has access to the environment, since you can use ?env:None so it probably makes sense to put it there.

talex5 · 2026-09-02T11:53:07Z

Now I'm wondering how useful it is to be able to mutable the environment globally like this. The uses I can think of are:

Changing the environment to be inherited by subprocesses. But then it would be safer to make a copy instead and pass it explicitly using Process.spawn ~env.
Passing values to other Eio code in the same process. But since you have to pass env around anyway, there's no advantage over passing the values directly. Or you could use fiber-local variables, which are typed and scoped sensibly.
Passing values to legacy/non-Eio code. But then the receiving code isn't using this system, so the mutex doesn't help, and you might as well use the Unix API directly.

It might be simpler if Stdenv.vars just returned the raw string array, and Eio_unix perhaps had some helpers to work with environments (e.g. convert to and from string maps, and convert values between strings and path lists).

patricoferris · 2026-09-02T12:29:05Z

It might be simpler if Stdenv.vars just returned the raw string array, and Eio_unix perhaps had some helpers to work with environments (e.g. convert to and from string maps, and convert values between strings and path lists).

I think this could be a fine approach, essentially making the environment as handled by Eio readonly (probably 90% of uses cases). It does not allow for @avsm's needs stated here #923 (comment) (though as you say, you can always default to the racier Unix.putenv should you need).

However, I think it would be nice to be able to write portable Eio applications that can Eio.Var.get_opt at the very least (without linking unix). For example, in a jsoo application where we can provide the environment from the jsooEnvvariable (see the documentation). Though, they have made that work with unix so maybe the point is a bit moot. I think this happens by default if Stdenv.vars returns a (string * string) list and users will naturally use List.assoc_opt or (for heavier use cases) transform it into a map (StringMap.of_list).

avsm · 2026-09-02T13:20:03Z

I agree it's useful to not have a dependency on Eio_unix for 'pure' Eio applications. However, I'm also happy with a read-only environment for now, given how horrendously messy mutating it is...

patricoferris · 2026-09-05T12:03:26Z

Thanks for adding those functions in #930 @talex5 !

Should the vars provided by the environment be a readonly copy of the vars when the program starts or should it be a function somewhat equivalent to:

let vars () = Unix.environment () |> Eio.Process.Env.of_array

so that it reflects Unix.putenv?

EDIT: I suspect it should be somewhat lazy. I'm running nix and my environment is pretty big (env | wc -c gives 14765). We probably shouldn't inflict that upon every Eio program?

talex5 · 2026-09-05T19:06:09Z

I think it should read the current environment each time you call it. Otherwise, there's no way to get that.

patricoferris · 2026-09-07T10:05:01Z

I have updated this PR to only contain the readonly functions that were here originally: get_all and get_opt. Initially, I had only provided direct access to the Eio.Process.Env.t, that is, the capability was method vars : unit -> Eio.Process.Env.t. This seems wasteful for the most likely use case which is to pick out a few environment variables for your application (e.g. XDG_DATA_DIRS).

Here are some quick benchmarks comparing the various APIs. The all benchmark calls get_allevery time and then finds a variable using Eio.Process.Env.get_opt, all_once calls get_all once and then Eio.Process.Env.get_opt every time and direct uses the Eio.Vars.get_opt directly every time:

                  name,   major-allocated,   minor-allocated,   monotonic-clock
        get_env/all 10,        346.619865,      38577.227215,      29248.828738
        get_env/all 50,       1677.003302,     181788.755923,     138307.102538
       get_env/all 100,       3323.770915,     361218.802888,     274768.577259
   get_env/all_once 10,        121.955078,      14694.900546,      11652.096583
   get_env/all_once 50,        563.133065,      61951.683658,      48161.605588
  get_env/all_once 100,       1113.238600,     121431.915770,      93960.646400
     get_env/direct 10,          0.000000,          0.000000,        974.649587
     get_env/direct 50,          0.000000,          0.000000,       4369.486360
    get_env/direct 100,          0.000000,          0.000000,       8753.393503

talex5 · 2026-09-07T16:18:13Z

Having a separate function for the common case of reading one variable seems fine to me.
(though a bit surprising that get_env/all_once is 10x slower than C, given that it should be doing about the same thing)

How about moving these two functions to the process manager? It makes sense to provide a way to get an Env.t from there, since you typically need it in order to add things before calling Process.spawn, and the process manager implicitly has access to it anyway (with ?env:None).

talex5 · 2026-09-07T16:43:33Z

(though a bit surprising that get_env/all_once is 10x slower than C, given that it should be doing about the same thing)

OK, I had a look at https://github.com/lattera/glibc/blob/master/stdlib/getenv.c and it's doing some tricks! It has a special case for 1-character names, and if not then it hard-codes the test for the first two characters for the fast-path!

(also String.starts_with would probably do better to call String.length prefix, which is fast, on each iteration, rather than allocating a closure to hold it)

talex5 · 2026-09-07T18:09:05Z

How about moving these two functions to the process manager?

To clarify, something like:

Eio.Process.environment : _ mgr -> Env.t
Eio.Process.getenv_opt : _ mgr -> string -> string option

patricoferris · 2026-09-08T11:38:04Z

Happy to add that. Are we not concerned that applications that just want to check the environment are also capable of spawning subprocesses?

talex5 · 2026-09-08T11:56:30Z

That is the down-side of moving it.

But, assuming the environment doesn't change, you can get the Process.Env.t snapshot at startup and pass that around just as easily as passing the separate Eio.Vars.t. And if you're mutating the environment later then it's probably because you want to spawn a process with it.

So I suspect having a separate Vars module isn't worth it (do you have a use for it?).

patricoferris · 2026-09-08T14:30:21Z

But, assuming the environment doesn't change, you can get the Process.Env.t snapshot at startup and pass that around just as easily as passing the separate Eio.Vars.t. And if you're mutating the environment later then it's probably because you want to spawn a process with it.

I'm convinced :-) I've added those functions in the latest push.

(though a bit surprising that get_env/all_once is 10x slower than C, given that it should be doing about the same thing)

Yes, I was a little surprised. Here is the benchmark when run inside a call to env - so all my Nix stuff isn't in there:

                  name,   major-allocated,   minor-allocated,   monotonic-clock
        get_env/all 10,          0.008949,        401.035481,        994.500899
        get_env/all 50,          0.038467,       1892.684723,       4655.264706
       get_env/all 100,          0.088574,       3866.065225,       9243.779433        
   get_env/all_once 10,          0.001615,        179.325236,        755.286173
   get_env/all_once 50,          0.026406,       1216.792224,       3414.363237
  get_env/all_once 100,          0.081945,       2738.526894,       6648.785586   
     get_env/direct 10,          0.000000,          0.000000,        415.258044
     get_env/direct 50,          0.000000,          0.000000,       1864.587791
    get_env/direct 100,          0.000000,        381.505302,       3698.969410

avsm reviewed Aug 19, 2026

View reviewed changes

Comment thread lib_eio/unix/vars.ml Outdated

Comment thread lib_eio/vars.ml Outdated

Comment thread tests/vars.md Outdated

avsm reviewed Aug 20, 2026

View reviewed changes

Comment thread lib_eio/eio.mli Outdated

avsm reviewed Aug 20, 2026

View reviewed changes

Comment thread lib_eio/vars.mli Outdated

avsm reviewed Aug 20, 2026

View reviewed changes

Comment thread lib_eio/vars.mli Outdated

patricoferris force-pushed the eio-system branch 2 times, most recently from 2887bce to 6b4f6d7 Compare August 21, 2026 11:12

avsm force-pushed the eio-system branch from 8872eea to 15018f8 Compare September 1, 2026 18:46

avsm approved these changes Sep 1, 2026

View reviewed changes

talex5 reviewed Sep 2, 2026

View reviewed changes

talex5 mentioned this pull request Sep 3, 2026

Add Process.Env #930

Merged

patricoferris force-pushed the eio-system branch from 15018f8 to 00e6d12 Compare September 7, 2026 09:23

Add environment functions to Eio.Process

859e639

patricoferris force-pushed the eio-system branch from 00e6d12 to 859e639 Compare September 8, 2026 14:26

	Eio.traceln "%s is %a" name Fmt.(quote string) (Eio.Vars.get t name)
	Eio.traceln "%s is %S" name (Eio.Vars.get t name)

		module Vars = Vars
		(** An {! Eio.System} interface using standard {! Unix} functions. *)


		(** {1 Accessing environment variables} *)

		val get_all : _ t -> (string * string) list

Uh oh!

Conversation

patricoferris commented Aug 19, 2026

Uh oh!

avsm left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

patricoferris commented Aug 19, 2026

Uh oh!

patricoferris commented Aug 20, 2026

Uh oh!

avsm commented Aug 20, 2026

Uh oh!

Uh oh!

Uh oh!

Uh oh!

patricoferris commented Aug 21, 2026

Uh oh!

avsm commented Aug 21, 2026

Uh oh!

patricoferris commented Sep 1, 2026

Uh oh!

avsm left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

talex5 commented Sep 2, 2026

Uh oh!

patricoferris commented Sep 2, 2026

Uh oh!

avsm commented Sep 2, 2026

Uh oh!

patricoferris commented Sep 5, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

talex5 commented Sep 5, 2026

Uh oh!

patricoferris commented Sep 7, 2026

Uh oh!

talex5 commented Sep 7, 2026

Uh oh!

talex5 commented Sep 7, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

talex5 commented Sep 7, 2026

Uh oh!

patricoferris commented Sep 8, 2026

Uh oh!

talex5 commented Sep 8, 2026

Uh oh!

patricoferris commented Sep 8, 2026

Uh oh!

Reviewers

Assignees

patricoferris commented Sep 5, 2026 •

edited

Loading

talex5 commented Sep 7, 2026 •

edited

Loading